Skip to content

fix(ui): harden Sheet focus handling#1217

Merged
BigSimmo merged 19 commits into
mainfrom
cursor/sheet-focus-hardening-c6d3
Jul 25, 2026
Merged

fix(ui): harden Sheet focus handling#1217
BigSimmo merged 19 commits into
mainfrom
cursor/sheet-focus-hardening-c6d3

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • The Sheet open-focus retry had grown into a polling loop (2s/50ms when this started, 10s/50ms on main now) because a closing sheet's focus restore had no way to know another sheet had opened — the new sheet had to fight the old sheet's restore back. This fixes the race at its source and makes the remaining settle work event-driven.
  • Guard the close-restore with the open-sheet stack. Restore proceeds when the target lives inside the top-most open sheet (a stacked sheet handing focus back down) and is skipped only when the target is out in the background behind a sheet that took over. Separate Sheet instances cannot cancel each other's timers, so the module-level stack is the only place this is knowable.
  • Clear this instance's own pending restore on open — the restoreTimersRef comment already claimed this, but the code no longer did it, so a close-then-reopen in one frame pulled focus back to the opener.
  • Replace the polling interval with a panel MutationObserver, a focusin listener, a stack subscription, and a short backoff chain. Idle when nothing changes; still upgrades from the close-button fallback to a late-mounted [data-sheet-autofocus] child the moment it mounts.
  • End the settle window on the first pointerdown/keydown so a deliberate interaction is never overridden, and bound reclaims that do not stick so a surface which re-focuses itself from its own focus handler cannot trade focus() calls with the controller until the stack blows. A reclaim that holds resets the count, so defending the panel repeatedly across the window is unaffected.
  • Only reclaim focus from nothing (document.body) or from background the sheet itself deactivated. A popover portaled out of the sheet (mode-action-popup, source-preview-popover, composer host) keeps focus.
  • Make the page background inert while the top-most sheet is open, so the browser refuses focus escape instead of the sheet correcting it. Skips live regions, the Next dev overlay/route announcer, data-sheet-inert-skip opt-outs, and any inert another owner set; only elements present at open are marked, so a surface portaled after open stays interactive.
  • Extract the stack, scroll lock, inerting, and focus controller into src/components/ui/sheet-focus.ts so the rules are unit-testable directly rather than only through a rendered Sheet.

Follow-up to the focus retries added in #1141. The merge from main keeps the case #1213 widened the poll for (a data-sheet-autofocus child arriving behind a slow dynamic import) by matching that settle window — listening is idle, so the longer window costs nothing.

Verification

  • npm run verify:pr-local — runtime, format, lint, typecheck, full unit suite, production build + client-bundle secret scan, and offline RAG fixture validation, all green on the final merged tree
  • npm run verify:ui (test:e2e:pr, Chromium) — 273 passed on the merged tree, including every toBeFocused assertion in the smoke spec
  • Flake soak: tests/ui-smoke.spec.ts --repeat-each=3 — 273 passed, no flake in the focus-heavy paths
  • Browser soak against the running app: 120 open/close cycles of the phone mode-menu sheet — zero inert-marker leaks, zero body.overflow drift, zero page errors, focus restored to the opener every cycle (median 0 ms, max 10 ms)
  • tests/sheet-focus-stress.dom.test.tsx (new, 13 cases): rapid open/close cycling, three-deep stacking, middle-sheet close, StrictMode double-invoked effects, sheets with no focusable target, document listener balance, inline (non-portal) inerting, detached roots, nested sheets, focus wars, and churn
  • tests/sheet-focus.dom.test.tsx (new): reclaim rules, background inerting including opt-out and foreign-inert cases, the sheet-swap restore race, close-then-reopen, late-mounted autofocus upgrade, user-interaction cancellation
  • tests/ui-accessibility.spec.ts (new case): proves the containment contract in a real browser — jsdom ignores inert, so this is the only place it can be proven
  • tests/ui-overlay-css-contract.test.ts pins "no setInterval polling" and the stack-guarded restore

What stress testing found

Two defects, both fixed here before this was ready for review:

  1. The first version of the stack guard skipped every restore while any sheet was open, so closing a stacked sheet dropped focus to the body instead of returning it to the sheet underneath. Caught by the three-deep stacking case.
  2. A background surface that re-focuses itself from its own focus handler could trade focus() calls with the controller synchronously. Now bounded, and the controller yields.

Risk and rollout

  • Risk: medium — background inerting changes what is interactive behind an open modal in real browsers (jsdom does not enforce inert). Focus behaviour itself is narrowed, not widened: the sheet steals focus in strictly fewer situations than before.
  • Rollback: revert this branch's commits; sheet.tsx returns to the in-file stack and the 50ms polling retry.
  • Provider or production effects: None

Notes

  • data-sheet-portal is a new opt-in marker for portaled surfaces that are logically inside a sheet; data-sheet-inert-skip is the background opt-out.
  • The dev server started for browser QA was stopped afterwards; worktree is clean and no repo-owned process was left running.
Open in Web Open in Cursor 

The open-focus retry had grown into a 2s/50ms poll because a closing
sheet's focus restore had no way to know another sheet had opened, so a
new sheet had to fight the old sheet's restore back. Fix the race at the
source and make the remaining settle work event-driven.

- Hold the close-restore behind the open-sheet stack. Instances cannot
  cancel each other's timers, so the stack is the only place a takeover
  is knowable.
- Clear this instance's own pending restore on open, which the
  restoreTimersRef comment already claimed but the code no longer did:
  a close-then-reopen in one frame pulled focus back to the opener.
- Replace the polling interval with a panel MutationObserver, a focusin
  listener, a stack subscription, and a short backoff chain. Idle when
  nothing changes, and it still upgrades to a late-mounted autofocus
  child.
- End the settle window on the first pointerdown/keydown so a deliberate
  interaction is never overridden.
- Only reclaim focus from nothing (body) or from background the sheet
  itself deactivated. A popover portaled out of the sheet keeps focus.
- Make the page background inert while the top-most sheet is open, so
  the browser refuses focus escape instead of the sheet correcting it.
  Skips live regions, dev overlays, opt-outs, and any inert another
  owner set; only elements present at open are marked, so a popover
  portaled after open stays interactive.
- Extract the stack, scroll lock, inerting, and focus controller into
  sheet-focus.ts so the rules are unit-testable directly.
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@cursor[bot], you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 75a4edf4-7f98-4572-871a-3c2603e62880

📥 Commits

Reviewing files that changed from the base of the PR and between 6d15121 and 65f7580.

📒 Files selected for processing (7)
  • docs/branch-review-ledger.md
  • src/components/ui/sheet-focus.ts
  • src/components/ui/sheet.tsx
  • tests/sheet-focus-stress.dom.test.tsx
  • tests/sheet-focus.dom.test.tsx
  • tests/ui-accessibility.spec.ts
  • tests/ui-overlay-css-contract.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/sheet-focus-hardening-c6d3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@supabase

supabase Bot commented Jul 25, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

github-actions Bot and others added 16 commits July 25, 2026 15:29
Stress testing found the open-sheet stack guard was too broad: it
skipped every restore while any sheet was open, so closing a stacked
sheet dropped focus to the body instead of handing it back to the sheet
underneath.

Restore now proceeds when the target lives inside the top-most open
sheet (stacked hand-back) and is skipped only when the target is out in
the background behind a sheet that took over.

Also bound how many times one open may reclaim focus. A surface that
re-focuses itself from its own focus handler would otherwise trade
focus() calls with the controller synchronously until the stack blew;
the controller now gives up and leaves focus where the other surface
put it.

Adds tests/sheet-focus-stress.dom.test.tsx: rapid open/close cycling,
three-deep stacking, middle-sheet close, StrictMode double effects,
no-focusable-target sheets, listener balance, inline (non-portal)
inerting, detached roots, nested sheets, focus wars, and churn.
jsdom ignores `inert`, so the modal containment contract had no
regression guard where it actually matters. Asserts that an open sheet
deactivates the page behind it, that the browser refuses focus into that
background, that the sheet subtree stays interactive, and that markers,
inert, scroll lock, and opener focus are all released on close.
main widened the open-focus poll to 200 attempts to defend a
`data-sheet-autofocus` child arriving behind a slow dynamic import (the
DocumentDrawer Find field) against focus=1 hydration and composer
reclaim. Resolved in favour of the event-driven controller, which
already upgrades from the close-button fallback the moment that child
mounts, and widened its settle window to match the case main was
covering — listening is idle, so the longer window costs nothing.

The reclaim budget now counts only reclaims that do not stick, so
repeatedly defending the panel across the window is unaffected while a
synchronous focus war still terminates.
Repeated pr-branch-sync bot merges left hosted CI in action_required on
the bot-authored heads. Applying skip-branch-sync plus this agent commit
so the required checks run under a non-bot identity before land.
@BigSimmo BigSimmo added the skip-branch-sync Opt out of hosted pr-branch-sync / update-branch on this PR label Jul 25, 2026 — with Cursor
@cursor
cursor Bot marked this pull request as ready for review July 25, 2026 17:30
@BigSimmo
BigSimmo enabled auto-merge (squash) July 25, 2026 17:39
@BigSimmo
BigSimmo merged commit 20300bb into main Jul 25, 2026
18 of 19 checks passed
@BigSimmo
BigSimmo deleted the cursor/sheet-focus-hardening-c6d3 branch July 25, 2026 17:39
cursor Bot pushed a commit that referenced this pull request Jul 25, 2026
…1233)

PR #1217 squash-merged to main as 20300bb with PR required green and
post-merge push CI green. Content verified on main: canRestoreFocusTo at
both restore call sites, bounded reclaims, the event-driven settle window
and no setInterval.

#82 previously recorded racing the sync bot as the only workaround.
Applying skip-branch-sync first stops the re-sync loop outright, which is
what let #1217 hold a green head long enough to merge.

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-branch-sync Opt out of hosted pr-branch-sync / update-branch on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants